Skip to content

Extract API to api package #253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mirromutth merged 1 commit into from
Mar 5, 2024
Merged

Extract API to api package #253

mirromutth merged 1 commit into from
Mar 5, 2024

Conversation

@mirromutth
Copy link
Collaborator

@mirromutth mirromutth commented Feb 29, 2024 •
edited
Loading

Motivation:

Extract public API to api package, see also #251

Modification:

Most of changes are to use strongly typed interfaces in the new api package instead of using r2dbc-spi interface directly. So, the modification seems large, but it is mainly changes such as implements ColumnMetadata -> implements MySqlColumnMetadata.

  • Extract API to api package
    • Use strongly typed interfaces
    • Change Codec/Codecs to use ReadableMetadata (MySqlReadableMetadata) instead of ColumnMetadata
    • Change the access modifier of non-public APIs to make them as private as possible, e.g. public class NonPublic -> [package-private] class NonPublic
    • Move MySqlStatement to api.MySqlStatement
    • Write more MySQL-specific API details in javadoc instead of inherited docs
    • Merge ColumnDefinition and MySqlTypeMetadata to new MySqlTypeMetadata, which is implemented by api.MySqlNativeTypeMetadata
  • Mark old MySqlTransactionDefinition as deprecated, use api.MySqlTransactionDefinition instead
  • Mark ConsistentSnapshotEngine as deprecated, use String instead of an enum
    • It seems like an over-engineered and hard to expand. e.g. A user wants to use an engine other than RocksDB or InnoDB, then the user needs to wait for us to add the element to the enum
    • This is a distribution-specific feature, we have no reason to maintain an enum for this
  • Find some bugs and add FIXME, they should be fixed in other tickets

Result:

Standardized our public interfaces

@mirromutth mirromutth added the enhancement New feature or request label Feb 29, 2024
@mirromutth mirromutth linked an issue Feb 29, 2024 that may be closed by this pull request
[feature] Extract public API into API package #251
Closed
@mirromutth mirromutth force-pushed the 251-feature-extract-public-api-into-api-package branch 3 times, most recently from 1d46f9d to 1810a33 Compare March 1, 2024 02:40
@mirromutth mirromutth marked this pull request as ready for review March 1, 2024 02:45
@mirromutth mirromutth requested a review from jchrys March 1, 2024 02:45
@mirromutth mirromutth force-pushed the 251-feature-extract-public-api-into-api-package branch 2 times, most recently from f5fee04 to 30735b1 Compare March 1, 2024 06:15
jchrys
jchrys reviewed Mar 1, 2024
View reviewed changes
@mirromutth
Extract public API to api package
62666b9 
- Use strongly typed interfaces
- Standardize R2DBC driver MySQL-specific API
@mirromutth mirromutth force-pushed the 251-feature-extract-public-api-into-api-package branch from 30735b1 to 62666b9 Compare March 1, 2024 11:33
@mirromutth mirromutth added this to the 1.1.3 milestone Mar 1, 2024
jchrys
jchrys approved these changes Mar 1, 2024
View reviewed changes
Copy link
Collaborator

@jchrys jchrys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mirromutth
Copy link
Collaborator Author

mirromutth commented Mar 2, 2024 •
edited
Loading

Oh, You're right. since this is not backward compatible(possible breaking compatibility). I think we need to increment major or minor. WDYT?

@jchrys

Those users who will not be affected by this modification:

  • Use this driver with spi ConnectionFactories
  • Only used MySqlConnectionConfiguration to configure somethings, and did not involve CodecRegistrar

This modification will cause a compilation warning for those users:

  • User is using MySqlTransactionDefinition and ConstantSnapshotEngine to use release-specific transaction properties. They should see @Deprecated

This modification will cause those users who need to modify the code to:

  • User is using CodecRegistrar to extend custom Codec. They should use MySqlReadableMetadata instead of the old MySqlColumnMetadata as any Codec should support OutParameters, which is a necessary change for the purpose of complying with the r2dbc spec
  • User is using old MySqlConnection, MySqlRow, MySqlStatement, etc. Their code may need to be changed to api.MySqlXxx, which makes sense because it was unclear which parts of the original API were public and which parts were not. Now with the api package, this will not be the case

I have 3 suggestions:

  • Consider a cumulative update to 1.2.0, which will provide clearly needed new features and fix all currently known issues. This should require more work
  • Just use 1.1.3, it is the first version for public API, and CodecRegitrar changes is necessary for supporting OUT parameters in r2dbc spec
  • Consider skipping some patch versions, e.g. 1.1.10 is next

@jchrys
Copy link
Collaborator

jchrys commented Mar 4, 2024 •
edited
Loading

Oh, You're right. since this is not backward compatible(possible breaking compatibility). I think we need to increment major or minor. WDYT?

@jchrys

Those users who will not be affected by this modification:

  • Use this driver with spi ConnectionFactories
  • Only used MySqlConnectionConfiguration to configure somethings, and did not involve CodecRegistrar

This modification will cause a compilation warning for those users:

  • User is using MySqlTransactionDefinition and ConstantSnapshotEngine to use release-specific transaction properties. They should see @Deprecated

This modification will cause those users who need to modify the code to:

  • User is using CodecRegistrar to extend custom Codec. They should use MySqlReadableMetadata instead of the old MySqlColumnMetadata as any Codec should support OutParameters, which is a necessary change for the purpose of complying with the r2dbc spec
  • User is using old MySqlConnection, MySqlRow, MySqlStatement, etc. Their code may need to be changed to api.MySqlXxx, which makes sense because it was unclear which parts of the original API were public and which parts were not. Now with the api package, this will not be the case

I have 3 suggestions:

  • Consider a cumulative update to 1.2.0, which will provide clearly needed new features and fix all currently known issues. This should require more work
  • Just use 1.1.3, it is the first version for public API, and CodecRegitrar changes is necessary for supporting OUT parameters in r2dbc spec
  • Consider skipping some patch versions, e.g. 1.1.10 is next

@mirromutth
It appears that we have yet to adopt a unified approach to versioning. It would be advantageous for us to agree on a versioning strategy. For example, whether to follow Strict SemVer (https://semver.org/) or a more relaxed version of SemVer, etc. In my opinion, I would like to suggest a slightly relaxed form of SemVer. For backward incompatible changes or new features, we could update the minor version. For backward compatible changes or new features, we could update the patch version. And for major versions, we could reserve those for significant or 'kaboom' changes. In this case, I would like to suggest updating to version 1.2.0.

@mirromutth
Copy link
Collaborator Author

mirromutth commented Mar 5, 2024

@jchrys Thanks.

I consider avoiding the version growing too fast, because actually this PR did not introduce much new content.

Maybe we should delay merging this PR until current known bugs are resolved. Then we consider upgrading to 1.2? WDYT?

@jchrys
Copy link
Collaborator

jchrys commented Mar 5, 2024 •
edited
Loading

@mirromutth
Thanks for sharing your thought.
In that case, proceeding with the originally planned version 1.1.3 seems appropriate.

Could you please confirm if the following version plan aligns with what you have in mind? if right we can follow that rule.

Major: Increment for big bang changes.
Minor: Increment when resolved all known bugs or for relatively big changes.
Patch: increment for everything else.

@mirromutth
Copy link
Collaborator Author

mirromutth commented Mar 5, 2024

LGTM, the major version increment of r2dbc-spi is also a big change

@jchrys
Copy link
Collaborator

jchrys commented Mar 5, 2024 •
edited
Loading

LGTM, the major version increment of r2dbc-spi is also a big change

Got it. I agree.

@mirromutth mirromutth merged commit 09a2246 into trunk Mar 5, 2024
@mirromutth mirromutth deleted the 251-feature-extract-public-api-into-api-package branch March 5, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jchrys jchrys jchrys approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

1.1.3

Development

Successfully merging this pull request may close these issues.

[feature] Extract public API into API package

3 participants

@mirromutth @jchrys